Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use HLM-provided land-use time series data to drive FATES harvest #663

Merged
merged 42 commits into from
Jul 29, 2020

Conversation

ckoven
Copy link
Contributor

@ckoven ckoven commented Jun 13, 2020

These changes provide a first version of land use change, as driven by the CLM & ELM landuse.timeseries files that currently drive the big-leaf versions of the models, and should allow global transient simulations with land use.

The idea is that the HLMs read a series of land-use driving datasets (as they already do), which they'll now then pass through the HLM-FATES interface via the interface variable bc_in%hlm_harvest(num_lu_harvest_cats). Currently the default is that the HLM reads and passes five area-based logging streams, for harvest from various categories of land (primary forest, primary nonforest, secondary young forest, secondary young nonforest, secondary old forest). FATES uses this data to harvest from three separate types: primary lands, secondary young lands, and secondary old lands. FATES does not distinguish between forest and no-forest lands, though possibly a future step could be to infer that either from the composition of a given patch or as specified if prescribed biogeography is turned on. Mass-based harvest and associated disturbance is not yet implemented here, that is also a next step.

The harvest parameters are kept from before, though their usage and meaning are slightly different.
The event code triggers when within the year to apply the harvest rates, the direct_frac determines what fraction of trees within the harvested area are removed (so fates_logging_direct_frac=1 means clearcut), etc. Plants within the logged area that are not harvested, whether because they are not trees, are not within the dbhmin threshold, or because (fates_logging_direct_frac + fates_logging_mechanical_frac + fates_logging_collateral_frac ) < 1 are all moved to a newly-disturbed secondary patch, along with an equivalent fraction of bare ground of each harvested patch if the canopy is not closed. So the definition of secondary lands includes everything from full clearcut disturbance to potentially relatively low-intensity forest degradation. But for consistency with the intention of the land-use datasets, we should probably set the default values to be closer to clear-cut type of conditions. Future steps may include making these arguments harvest-type-dependent. Also we still need to wire the max_dbh parameter introduced in #659 to this logic, once that code is merged into this.

We added a bunch of diagnostics to serve as either sanity checks or other trackers of things that might be useful, including tracking various disturbance rates, as well as 'potential disturbace' rates (i.e. the disturbance rates before the part of the code where it says only resolve a given patch's most dominant disturbance type on a given day). Also changed the newly-disturbed patch insertion to fix a bug as described in #660.

This isn't really ready to be merged just yet, in particular the merging with the prescribed biogeography (#632 and follow-on) will most likely be a bit tricky. But putting up for interested parties to start reviewing.

fixes: #660, #491, #276, and the first batch of changes described in #450.

Collaborators:

co-written by @aldivi and me. @aldivi also wrote the E3SM-side changes of these (PR to come later) and I wrote the CTSM-side changes (PR ESCOMP/CTSM#1040).

Expectation of Answer Changes:

This should only change answers when logging is operational. But this hasn't been formally tested yet and probably requires some new tests that use the land-use timeseries data.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the in-code documentation .AND. (the technical note .OR. the wiki) accordingly.
  • I have read the CONTRIBUTING document.
  • FATES PASS/FAIL regression tests were run
  • If answers were expected to change, evaluation was performed and provided

Test Results:

CTSM (or) E3SM (specify which) test hash-tag:

CTSM (or) E3SM (specify which) baseline hash-tag:

FATES baseline hash-tag:

Test Output:

aldivi and others added 23 commits February 9, 2020 08:24
update my fork with master
update my fork with original
Updated fates interface to receive harvest data from hlm.
This includes flags denoting whether to use hlm harvest
or fates logging, and whether the hlm harvest is area or carbon.
Have not implemented the actual harvest yet, and plan to use
fates logging parameters to direct it.

 Changes to be committed:
	modified:   biogeochem/EDLoggingMortalityMod.F90
	modified:   main/FatesInterfaceMod.F90
Completed the initial code for passing land use harvest from
ELM to FATES. Still need to debug and test.
 Changes to be committed:
	modified:   biogeochem/EDLoggingMortalityMod.F90
	modified:   main/FatesConstantsMod.F90
	modified:   main/FatesInterfaceMod.F90
 Changes to be committed:
	modified:   biogeochem/EDLoggingMortalityMod.F90
	modified:   biogeochem/EDMortalityFunctionsMod.F90
	modified:   biogeochem/EDPatchDynamicsMod.F90
 Changes to be committed:
	modified:   biogeochem/EDLoggingMortalityMod.F90
	modified:   main/FatesInterfaceMod.F90
The harvest data from landuse.timeseries are passed to FATES for
logging. The FATES logging parameters are used to guide harvest.
Only area fraction data are implemented, and these fractions are
applied to the FATES logging intensity parameters. The desired
effect is for the given logging intensity to be applied to a fraction
of the cohort determined by the HLM harvest rate. Separate fractions
are applied to primary and secondary patches. This is a working version
for testing.
 Changes to be committed:
	modified:   biogeochem/EDLoggingMortalityMod.F90
…ter all the secondary patches, and error-check if the code tries to merge patches with different labels
Conversion to secondary land due to harvest is based on the area
harvested, not just the logged crown. Also, the harvest_carbon_flux
diagnostic has been updated to align with the wood_product output.
Note that these are in different units and that monthly average
output during harvest month is different for each.
 Changes to be committed:
	modified:   biogeochem/EDLoggingMortalityMod.F90
	modified:   biogeochem/EDPatchDynamicsMod.F90
main/FatesInterfaceTypesMod.F90 Outdated Show resolved Hide resolved
main/FatesInterfaceTypesMod.F90 Outdated Show resolved Hide resolved
biogeochem/EDLoggingMortalityMod.F90 Outdated Show resolved Hide resolved
biogeochem/EDLoggingMortalityMod.F90 Show resolved Hide resolved
main/FatesConstantsMod.F90 Outdated Show resolved Hide resolved
@ckoven
Copy link
Contributor Author

ckoven commented Jul 1, 2020

some note from a discussion with @rgknox on this PR:

(1) right now, the CTSM code is assuming that the harvest is area-based (see here: https://github.com/ckoven/ctsm/blob/fates_landuse/src/utils/clmfates_interfaceMod.F90#L340). the problem is that right now, the way we are specifying the units happens early in the call sequence. Instead, the suggestion is to change this to make sure that the units that are read in (here: https://github.com/ckoven/ctsm/blob/fates_landuse/src/biogeochem/dynHarvestMod.F90#L124) are passed through the interface alongside the rates (also rename these as per #663 (comment)). At some later point we can update that to use a parameter interface that @rgknox has developed for the nutrient code to handle a similar problem, which should be more efficient.

(2) at some point we should add some checks to ensure that parameter values make sense. in particular, the harvest timeseries code only really makes sense for certain values of the
need to add a new subroutine to check these constants (like that it is less than 1000). call this subroutine fates_checkscalarparams, called from fatesinterfacemod after here: https://github.com/NGEET/fates/blob/master/main/FatesInterfaceMod.F90#L1410 and like the checks that happen before it.

(3) a few little things, e.g. change name of use_history to patch_ anthro_disturbance_label here: https://github.com/ckoven/fates/blob/fates_harvest_offmaster/biogeochem/EDLoggingMortalityMod.F90#L315

@rgknox
Copy link
Contributor

rgknox commented Jul 2, 2020

@ckoven , I'm noticing an issue that may crop up now that we are using logging with gridded runs. We currently determine if it is "logging time" by evaluating the date, but only on the master node. In a gridded run, the worker threads may not even get evaluated, and/or get to the calculation of the areas before the master updates the global "logging time".

https://github.com/NGEET/fates/blob/master/biogeochem/EDLoggingMortalityMod.F90#L86

I have to look more at how this routine is used, but it might be good to change "logging time" to a site level variable (not a global) and have this evaluated and updated on all threads.

After looking at the code more, I realize that we do calculate logging time on all threads, we only use the pointer to the main thread to filter out print statements to prevent redundant messaging. This call may be inefficient (although it is incredibly cheap compared to other operations its not worth improving) but I don't think there is anything wrong anymore.

@rgknox rgknox requested a review from KatieMurenbeeld July 2, 2020 14:49
@@ -55,6 +58,12 @@ module EDLoggingMortalityMod
use PRTGenericMod , only : sapw_organ, struct_organ, leaf_organ
use PRTGenericMod , only : fnrt_organ, store_organ, repro_organ
use FatesAllometryMod , only : set_root_fraction
use FatesConstantsMod , only : primaryforest, secondaryforest, secondary_age_threshold

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An idea for down the road, in the US Forest Service most harvesting/logging is done with a specific age class outcome as an object. The classes are: even-age (one age class, total range of ages present <20% of the rotation age), two-aged (two age classes), and uneven-aged (three or more distinct age classes), all of which have their own harvest practices associated with them. This may be useful designation in the future as more logging/harvest features are added.

lmort_direct = 0.0_r8
l_degrad = logging_direct_frac * adjustment
lmort_direct = 0.0_r8
l_degrad = harvest_rate

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that if trees are not large enough to log, the cohort is still degraded/moved to secondary patch?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, never mind I see, background degradation rate regardless of logging.

@ckoven
Copy link
Contributor Author

ckoven commented Jul 8, 2020

@aldivi and I have had a bunch of discussions in the past days with various stakeholders (@JoshuaRady @KatieMurenbeeld @lawrencepj1 @dlawrenncar @wwieder @ekluzek) and I think that all are okay with this PR as it currently stands, so I think it is ready to test and then merge if tests pass.

@lawrencepj1
Copy link

lawrencepj1 commented Jul 8, 2020 via email

@glemieux
Copy link
Contributor

glemieux commented Jul 10, 2020

Regression tests underway

UPDATE: @ckoven everything looks good except that the FatesLogging regression test is running into a mass balance error. Taking a look into it now. Run logs are here:

/glade/u/home/glemieux/scratch/clmed-tests/pr663-landuse-Cc59f7d61-F8d9f189b.fates.cheyenne.intel/ERS_Ld60.f45_f45_mg37.I2000Clm50FatesCruGs.cheyenne_intel.clm-FatesLogging.C.20200710_111225_d0itwv/run

If I'm reading the diagnostic output from the log correctly it looks like its failing on secondary forest and the disturbance_mode is currently unset

@glemieux glemieux self-assigned this Jul 10, 2020
@ckoven
Copy link
Contributor Author

ckoven commented Jul 10, 2020

noting here some diagnostics based on a global 4x5 historical tranisent run that I made in a jupyter notebook here. The notebook shows some issues that needed to be worked out on the version of the code that I used for the run, which I believe to have been corrected since, but I am also noting here that while I think this code is now ready to be merged I want to return to these diagnostics at a later date once we are using the raw LUH2 as the driver (and also have better global productivity and biomass behavior).

https://github.com/ckoven/landuse_runscripts/blob/master/landuse_global_analysis.ipynb

@glemieux
Copy link
Contributor

Ran regression test on update post bugfixes. All expected PASS. Only differences due to new history variables added and answer changing results for the FatesLogging test which is expected. There is also an expected difference in the SITE_DAYSINCE_COLDLEAFOFF (and related) that is due to reinstating CTSM PR 820 which was accidentally dropped during updating fates_next_api to relase-clm5.0.30. Test results located here:

/glade/u/home/glemieux/scratch/clmed-tests/pr663-landuse-C39a5a119-F8872a912.fates.cheyenne.intel
/glade/u/home/glemieux/scratch/clmed-tests/pr663-landuse-C39a5a119-F8872a912.fates.cheyenne.gnu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

current code not conserving primary/second land areas
7 participants